Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Iceberg writer stage should include WRITER_MAXIMUM_POOL_SIZE in its parameters #170

Merged
merged 1 commit into from
Apr 13, 2022
Merged

Iceberg writer stage should include WRITER_MAXIMUM_POOL_SIZE in its parameters #170

merged 1 commit into from
Apr 13, 2022

Conversation

liuml07
Copy link
Contributor

@liuml07 liuml07 commented Apr 13, 2022

Context

Iceberg writer stage missed to add WRITER_MAXIMUM_POOL_SIZE into the parameter list. So all overrides in jobs will be ignored and the default value will be used regardless.

Checklist

  • ./gradlew build compiles code correctly
  • Added new tests where applicable
  • ./gradlew test passes all tests
  • Extended README or added javadocs where applicable
  • Added copyright headers for new files from CONTRIBUTING.md

@liuml07
Copy link
Contributor Author

liuml07 commented Apr 13, 2022

Trying to add a unit test but the patch 1) straightfoward 2) not easy to valid such coding omission. Perhaps we can add some test in spring-boot module after this is merged if necessary.

@calvin681
Copy link
Collaborator

You may want to back port this to the release-1.3 branch.

@sundargates
Copy link
Collaborator

@liuml07 It looks like we should be using Parameters::get without the default value in this case. That would surface the runtime exception and hence the problem in this case.

public Object get(String key) {
        // check if required parameter, make sure
        // has a value
        if (requiredParameters.contains(key) &&
                !state.containsKey(key)) {
            throw new ParameterException("Attempting to reference a required parameter witn no value: " + key + ", check parameter definitions for job.");
        }
        // check if parameter definition exists
        if (!parameterDefinitions.contains(key)) {
            throw new ParameterException("Attempting to reference parameter: " + key + ", with no definition, check parameter definitions in job.");
        }
        return state.get(key);
    }

    public Object get(String key, Object defaultValue) {
        Object value = defaultValue;
        try {
            value = get(key);
            if (value == null) {
                value = defaultValue;
            }
        } catch (ParameterException ex) {
            value = defaultValue;
        }
        return value;
    }

codyrioux
codyrioux approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@codyrioux codyrioux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Sundaram in that we should not be using a default at the call site where we fetch the parameter.

@liuml07
Copy link
Contributor Author

liuml07 commented Apr 13, 2022

Makes sense. I'll file a follow-up PR to change all callsites in Iceberg sink module.

@liuml07 liuml07 merged commit 6f42639 into master Apr 13, 2022
@liuml07 liuml07 mentioned this pull request Apr 13, 2022
Merged
5 tasks
@Andyz26 Andyz26 deleted the liuml07/iceberg-sink-writer-maxpoolsize branch August 1, 2023 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codyrioux codyrioux codyrioux approved these changes

@calvin681 calvin681 calvin681 approved these changes

@nickmahilani nickmahilani Awaiting requested review from nickmahilani

@jeffchao jeffchao Awaiting requested review from jeffchao

@piygoyal piygoyal Awaiting requested review from piygoyal

@sundargates sundargates Awaiting requested review from sundargates sundargates is a code owner

@Andyz26 Andyz26 Awaiting requested review from Andyz26 Andyz26 is a code owner

@hmitnflx hmitnflx Awaiting requested review from hmitnflx hmitnflx is a code owner

@markcho markcho Awaiting requested review from markcho

Assignees
No one assigned
Labels
iceberg
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liuml07 @calvin681 @sundargates @codyrioux